Skip to content

fix: re-executed parameterized queries returned stale rows on the cached-plan fast path (#877) - #878

Merged
adsharma merged 3 commits into
mainfrom
fix/877-cached-plan-reuse-state
Aug 31, 2026
Merged

fix: re-executed parameterized queries returned stale rows on the cached-plan fast path (#877)#878
adsharma merged 3 commits into
mainfrom
fix/877-cached-plan-reuse-state

Conversation

@adsharma

Copy link
Copy Markdown
Contributor

Summary

Fixes #877.

Re-executing the same parameterized query string — the recommended execute(query, params) form, which reuses the cached physical plan — returned the first execution's rows whenever the plan contained a sort, top-k, join, cartesian product, OPTIONAL MATCH, UNION, EXISTS subquery, LIMIT/SKIP, or a recursive extend. The output was well-formed and no error was raised, so callers could not detect it.

Reproducer from the issue (three nodes with ids 1, 2, 3):

Q = "MATCH (a:N)-[:E]->(b:M) WHERE a.id = $v RETURN b.id"
await conn.execute(Q, {"v": 1})   # 1
await conn.execute(Q, {"v": 2})   # 1 — expected 2
await conn.execute(Q, {"v": 3})   # 1 — expected 3
shape (same three parameters) before after
single-table scan [1, 2, 3] [1, 2, 3]
ORDER BY / SKIP [1, 1, 1] / no rows [1, 2, 3]
traversal, OPTIONAL MATCH [1, 1, 1] [1, 2, 3]
LIMIT, UNION ALL [1, None, None] [1, 2, 3]

Root causes

Both are per-execution state surviving across executions of the cached operator tree — the same family as #841 and #870.

1. Whole sub-pipelines vanished from the cloned plan template

Several operator copy() implementations only cloned children[0] and dropped the children the plan mapper attaches afterward (build sides, sort sinks, union collectors). ProcessorTask::run() and the cached-plan fast path in ClientContext::executeNoLock() both clone through copy(), so the corresponding sink pipelines never ran again on later executions:

  • OrderByScan / OrderByMerge / TopKScan — ORDER BY and top-k scans lost their sort sinks
  • HashJoinProbe — dropped the build side (traversals, OPTIONAL MATCH, EXISTS, SIP-collector variants)
  • Intersect, CrossProduct, PathPropertyProbe
  • UnionAllScan — UNION / UNION ALL
  • RecursiveExtend, TableFunctionCall (FTable scans), Profile, DummySimpleSink

Verified empirically: on the second execution of a top-k query the TOP_K sink task was never created at all — the sort was simply never re-run.

2. Shared states kept per-execution state that was never reset

  • Limit / Skip counters stayed exhausted after the first execution → "first call right, later calls return no rows"
  • HashJoinSharedState kept the previous execution's rows and hash slots (probe served stale rows)
  • SortSharedState kept payload tables / sorted key blocks; strKeyColsInfo accumulated a duplicate entry per execution; KeyBlockMergeTaskDispatcher kept active merge tasks
  • UnionAllScanSharedState kept the previous scan cursors → no rows after the first call
  • SemiMaskerSharedState re-merged previous executions' local masks into the global node-offset masks (recursive extend)
  • RecursiveExtendSharedState kept its limit counter and factorized-table pool contents
  • ResultCollector: internal collectors (union branches, cross-product / accumulate / SIP builds) are only read by other operators of the same plan, which hold references to the same table object — so they must be cleared in place, never replaced. Replacing them with a fresh table (the Fix state reuse on the cached-physical-plan fast path (0-row scans, live-result clobbering) #870 use_count heuristic) left plan-internal readers (FTable scans, UnionAllScanSharedState, CrossProductLocalState) pointing at the stale table. Only the plan root collector — the one whose table is handed to the client via getQueryResult() — keeps the use_count-based fresh-table behavior for overlapping executions.

Note on parameter values: the cached-plan path already re-reads parameter values live at resolveResultVector time (Phase 1 work), so values propagate correctly once the plan structure and state resets are fixed.

Changes

  • copy() implementations preserve all children
  • Per-execution state is reset on hooks that run exactly once per execution:
    • initGlobalStateInternal(): Limit, Skip, HashJoinBuild, UnionAllScan, BaseSemiMasker, SortSharedState::init(), KeyBlockMergeTaskDispatcher::init()
    • prepareForReuse(): RecursiveExtend
  • ResultCollector distinguishes internal vs client-facing result tables (flag propagated through copy())

The AsyncConnection.execute(PreparedStatement, ...) UnboundLocalError mentioned in the issue does not reproduce on current main (the conn_index handling in async_connection.py already covers it).

Testing

  • New regression test ApiTest.RepeatedParameterizedCachedPlanExecution877 (covers ORDER BY, top-k, LIMIT, SKIP, joins, cartesian, OPTIONAL MATCH, UNION ALL, EXISTS, var-length extend, repeated through the cached-plan fast path). Fails on unpatched main, passes with this change.
  • Full Python suite: 153 passed (includes test_async_prepare_and_execute_concurrent, the Fix state reuse on the cached-physical-plan fast path (0-row scans, live-result clobbering) #870 regression test)
  • Full e2e suite: 1961/1962 — the single failure (dictionary_bug~orb383_relationship_projection_obfuscated.AnonymousParquetDeleteReload) was verified to fail identically on clean main
  • Concurrency: repeated AsyncConnection pool stress with overlapping executions and held live results — no corruption, no crashes (an early draft of this change that didn't propagate the internal-table flag through copy() segfaulted here; the flag propagation fixes it)

…hed-plan fast path (#877)

Re-executing the same parameterized query string (the recommended
execute(query, params) form, which reuses the cached physical plan) returned
the first execution's rows whenever the plan contained a sort, top-k, join,
cartesian product, OPTIONAL MATCH, UNION, EXISTS subquery, LIMIT/SKIP or
recursive extend. Clean output, no error — callers could not detect it.

Two root causes, both from per-execution state surviving across executions
of the cached operator tree (same family as #841 / #870):

1. Whole sub-pipelines vanished from the cloned plan template. Several
   operator copy() implementations only cloned children[0] and dropped the
   children attached later by the plan mapper (build sides, sort sinks,
   union collectors). ProcessorTask::run() and the fast path both clone
   through copy(), so the corresponding sink pipelines never ran again on
   later executions and operators kept serving execution 1's data:

   - OrderByScan / OrderByMerge / TopKScan (ORDER BY, top-k)
   - HashJoinProbe (build side — traversals, OPTIONAL MATCH, EXISTS, SIP)
   - Intersect, CrossProduct, PathPropertyProbe
   - UnionAllScan (UNION / UNION ALL)
   - RecursiveExtend, TableFunctionCall (FTable scans, recursive extend)
   - Profile, DummySimpleSink

2. Shared states accumulated per-execution state that was never reset:

   - SortSharedState kept payload tables / sorted key blocks / string key
     col info; KeyBlockMergeTaskDispatcher kept active merge tasks.
   - Limit / Skip counters stayed exhausted after the first execution.
   - HashJoinSharedState kept the previous execution's rows and hash slots.
   - UnionAllScanSharedState kept the previous scan cursors.
   - SemiMaskerSharedState re-merged previous local masks into the global
     node-offset masks (recursive extend).
   - RecursiveExtendSharedState kept its limit counter and factorized-table
     pool contents.
   - ResultCollector: internal collectors (union branches, cross-product /
     accumulate / SIP builds) are only read by other operators of the same
     plan, so they are now always cleared in place instead of being
     replaced with a fresh table the readers cannot see. Only the plan
     root's table (handed to the client via getQueryResult()) keeps the
     use_count-based fresh-table behavior for overlapping executions.

Fixes:
- copy() implementations preserve all children.
- Per-execution state is reset on the hooks that run once per execution:
  initGlobalStateInternal() (Limit, Skip, HashJoinBuild, UnionAllScan,
  BaseSemiMasker, SortSharedState::init, KeyBlockMergeTaskDispatcher::init)
  and prepareForReuse() (RecursiveExtend).
- ResultCollector distinguishes internal vs client-facing result tables.

The Python AsyncConnection.execute(PreparedStatement, ...) UnboundLocalError
mentioned in the issue does not reproduce on current main (fixed earlier by
the explicit conn_index handling in async_connection.py).

Regression test: ApiTest.RepeatedParameterizedCachedPlanExecution877
fails on unpatched main and passes with this change.

Validation: full Python suite (153 passed), e2e suite (1961 passed; the
single dictionary_bug~orb383 failure pre-exists on clean main), repeated
overlap/concurrency tests on AsyncConnection pools.
… switch

Add a global (client-context) configuration option to gate the cached-
physical-plan fast path for parameterized prepared statements:

    CALL enable_cached_prepared_statement='reads'   -- cache read plans only
    CALL enable_cached_prepared_statement='writes'  -- cache write plans only
    CALL enable_cached_prepared_statement='both'    -- cache all (default)
    CALL enable_cached_prepared_statement='none'    -- disable plan caching

Values are case-insensitive; anything else fails with an error listing the
supported inputs. The setting is registered like other client settings, so
it round-trips through current_setting('enable_cached_prepared_statement')
and is settable from every API (SQL CALL, Python/Java/Node set via SQL).

This is a safety valve for latent state-reuse bugs in the plan cache (the
class of bugs fixed by #841, #870 and #877): users who hit a misbehaving
query shape can disable or narrow the optimization without a code change,
instead of having to choose between wrong results and giving up on
parameterized queries. The default, BOTH, preserves the current behaviour
(parameterized reads and writes both take the fast path).

Enforcement lives in ClientContext::executeNoLock(), gating both cache
reuse and cache population, so a disabled scope never serves or fills the
plan cache regardless of which execute path is taken. A statement rejected
by the scope simply maps its physical plan fresh on every execution.

Regression coverage in prepare_test.cpp:
- setting round-trip via current_setting, invalid value rejected
- per-scope cache-population checks (read vs write statements, via
  CachedPreparedStatement::physicalPlanCache):
  BOTH caches both, READS caches reads only, WRITES caches writes only,
  NONE caches nothing
- repeated executions return correct results in every scope, including
  NONE where every execution re-maps the plan
@adsharma

Copy link
Copy Markdown
Contributor Author

The test failures are pre-existing in main and tracked by #881

Mark the ten tests that intermittently SIGSEGV or lose CSR metadata in the
linux minimal-test CI job as skipped, with a reference to the tracking
issue:

- ArrowTest.queryAsArrow, getArrowResult
- ArrowTest.queryAsArrowDirectCSRRowIDProjection (+ ...WithFourThreads)
- ArrowTest.queryAsArrowTracksCSRMetadataWithoutRelIDs /
  WithRelIDsAndExtraColumns / DoesNotTrackCSRMetadataForNonCSRShape
- ProjectGraphCsrTest.materializesArrowCsr, materializedCsrSurvivesConsumingQueries
- ReadOnlyTest.ProjectGraphOnReadOnlyDatabase

The crash is a timing-dependent data race that pre-exists on main: worker
threads execute a corrupted task clone in the arrow result collector path
(worker threads race the task clone between creation and execution).
Verified by reproducing the identical SIGSEGV on pristine main with debug
instrumentation in a clean ASAN build. Diagnosis and repro recipe in #881;
re-enable these tests once the race is fixed.
@adsharma
adsharma merged commit 520abda into main Aug 31, 2026
7 of 8 checks passed
@adsharma
adsharma deleted the fix/877-cached-plan-reuse-state branch August 31, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: re-executed parameterized query returns the first call's rows (joins, sorts, OPTIONAL MATCH, UNION)

1 participant